Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 2, 2019

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the interp module as far as Miri is concerned -- with the exception of the uninit intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.

@rust-highfive
Copy link
Contributor

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 2, 2019
@cramertj
Copy link
Member

cramertj commented Nov 4, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 4, 2019

📌 Commit 0aee0dd has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2019
@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2019

does priroda need these methods?

Yes, it needs get to print an allocation: https://github.com/oli-obk/priroda/blob/c6a4126b2bf721716bf5336dd0d848f6780d4d53/src/render/locals.rs#L350

@RalfJung
Copy link
Member Author

RalfJung commented Nov 4, 2019

@bjorn3 okay, so we should likely still allow immutable access. But we can shut down mutable access eventually.

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2019

I have actually been thinking about allowing the user to mutate state (including allocations) instead of just observing it. However I havent worked on priroda anymore for 5 months according to github.

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2019

Maybe renaming the functions to _get_raw and _get_raw_mut will discourage usage even more. This is already used for _intern_substs and several other interners which get wrapped by the function without _ as prefix.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 4, 2019

For now I don't think things are bad enough that we need to resort to leading underscores...

Centril added a commit to Centril/rust that referenced this pull request Nov 5, 2019
rename Memory::get methods to get_raw to indicate their unchecked nature

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the `interp` module as far as Miri is concerned -- with the exception of the `uninit` intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.
@Centril
Copy link
Contributor

Centril commented Nov 6, 2019

Toolstate failure in #66138 (comment), @bors rollup=never p=-1

Please revert ^-- in a bit.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 6, 2019

A Miri failure shouldn't be a problem even during the beta week...?

@Centril
Copy link
Contributor

Centril commented Nov 6, 2019

🤷‍♂ -- beats me; it seems to have caused a failure tho... but I think toolstate blockage is finally over.

@bors p=0 rollup=maybe

@RalfJung
Copy link
Member Author

RalfJung commented Nov 6, 2019

Ah, clippy-driver also got broken in that rollup. Whether that was this PR or another one I don't know. I guess we'll see.^^

@RalfJung
Copy link
Member Author

RalfJung commented Nov 6, 2019

Looks like clippy was affected by another PR in the rollup: #66150

@bors
Copy link
Collaborator

bors commented Nov 7, 2019

☔ The latest upstream changes (presumably #66175) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 7, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Nov 8, 2019

Rebased.

@bors r=cramertj

@bors
Copy link
Collaborator

bors commented Nov 8, 2019

📌 Commit 15ec8f7 has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2019
@bors
Copy link
Collaborator

bors commented Nov 8, 2019

⌛ Testing commit 15ec8f7 with merge b21093f8fcb2591907a739fd7a05cd8500727521...

Centril added a commit to Centril/rust that referenced this pull request Nov 8, 2019
rename Memory::get methods to get_raw to indicate their unchecked nature

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the `interp` module as far as Miri is concerned -- with the exception of the `uninit` intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.
@Centril
Copy link
Contributor

Centril commented Nov 8, 2019

@bors retry rolled up.

Centril added a commit to Centril/rust that referenced this pull request Nov 8, 2019
rename Memory::get methods to get_raw to indicate their unchecked nature

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the `interp` module as far as Miri is concerned -- with the exception of the `uninit` intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.
bors added a commit that referenced this pull request Nov 8, 2019
Rollup of 5 pull requests

Successful merges:

 - #65785 (Transition future compat lints to {ERROR, DENY} - Take 2)
 - #66007 (Remove "here" from "expected one of X here")
 - #66043 (rename Memory::get methods to get_raw to indicate their unchecked nature)
 - #66154 (miri: Rename to_{u,i}size to to_machine_{u,i}size)
 - #66188 (`MethodSig` -> `FnSig` & Use it in `ItemKind::Fn`)

Failed merges:

r? @ghost
@bors bors merged commit 15ec8f7 into rust-lang:master Nov 8, 2019
@RalfJung RalfJung deleted the memory-get-raw branch November 9, 2019 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants